Murisi/integrate hw masp #3797

murisi · 2024-09-10T10:44:54Z

Describe your changes

Made the following changes to enable hardware wallet signing of MASP Transactions:

Allow the outsourcing of generator randomness to the hardware wallet
Allow the generation of MASP extended full viewing keys using the hardware wallet
Allow the client to generate proofs using proof generation keys from the hardware wallet
Now apply signatures generated by the hardware wallet onto client build Transactions
The following transaction types are supported by MASP hardware wallet signing:
- Shielding transactions (only randomness parameters are taken from hardware wallet)
- Shielded, unshielding, and IBC transactions (randomness parameters and proof generation keys are taken from hardware wallet)
- Combined hardware wallet and software client signing where the the shielded keys are on the hardware wallet and wrapper signer keys are on the computer, or vice-versa, or both reside on the device is supported
Adjusted the hardware and localnet genesis balances files in order to ensure that the MASP inflation controller computations are the same for both kinds of tests
Recomputed/recalibrated the MASP test numerical expectations to work with the new adjusted balances files
Modified the MASP test commands to enable signing to happen on the hardware wallet when NAMADA_E2E_USE_DEVICE=true
Moved the MASP spending and viewing keys as well as payment addresses used in the the tests crate into the localnet and hardware genesis files to enable hardware wallet signing
Separated spending and viewing keys from their birthdays in the wallet's store.toml in order to ease the manual editing of these files (as done above)
Removed the possibility of using the hardware wallet to sign the wrapper header without signing the raw header because the former depends on the latter in the current design
Now ensure the removal of the Builder section in sign_tx because it contains private information and because it will be rejected on the basis of not being referred to in the wrapper signature section

This PR depends upon the following being merged:

Murisi/box impl build params masp#88 to reduce the constraints induced by the Builder API
Murisi/zip32 support Zondax/ledger-namada#61 to generate extended full viewing keys.
Fix signing for non-MASP IBC Transfers heliaxdev/ledger-namada#2 to fix the signing of pure IBC Txs

Outstanding issues:

The hardware wallet applies ZIP 32 derivations to a seed obtained from doing SLIP 10 derivation with a fixed path. (This was necessary because the hardware wallet's ZIP 32 functions are unable to directly access the root seed.) This PR currently modifies namadaw's key derivation to match the hardware wallet's to ease checking that the same extended keys are derived for a given mnemonic. However, it might be desirable to revert namadaw's derivation to pure ZIP 32 if we desire interoperability with systems implementing this standard strictly. Or perhaps both types of ZIP 32 could be supported with a namadaw CLI flag to choose which derivation is desired. See a relevant discusssion at https://zecsec.com/audits/zcash-ledger-audit-report-v2.pdf#subsection.3.2 .

Checklist before merging

If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
- This will require 2 reviewers to approve the changes

Fraccaman · 2024-09-18T13:02:00Z

@murisi thanks for PR! Do u mind rebasing this on main?

codecov · 2024-09-23T10:53:46Z

Codecov Report

Attention: Patch coverage is 50.31447% with 79 lines in your changes missing coverage. Please review.

Project coverage is 72.81%. Comparing base (8d11769) to head (0332a63).
Report is 41 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/wallet/src/lib.rs	24.32%	28 Missing ⚠️
crates/wallet/src/store.rs	39.39%	20 Missing ⚠️
crates/core/src/masp.rs	12.50%	7 Missing ⚠️
crates/sdk/src/signing.rs	33.33%	6 Missing ⚠️
crates/sdk/src/tx.rs	75.00%	5 Missing ⚠️
crates/sdk/src/lib.rs	0.00%	4 Missing ⚠️
crates/tx/src/types.rs	84.21%	3 Missing ⚠️
crates/apps_lib/src/config/genesis/transactions.rs	0.00%	2 Missing ⚠️
crates/apps_lib/src/config/genesis/utils.rs	0.00%	2 Missing ⚠️
crates/sdk/src/args.rs	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3797      +/-   ##
==========================================
- Coverage   72.83%   72.81%   -0.03%     
==========================================
  Files         338      338              
  Lines      104220   104306      +86     
==========================================
+ Hits        75907    75946      +39     
- Misses      28313    28360      +47

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tzemanovic · 2024-09-26T10:33:19Z

I'm trying to run e2e::ledger_tests::masp_txs_and_queries with these changes and 713f954:

Something is wrong with the wrapper signature. It fails on shield --source Albert --target znam1ky620tz7z658cralqt693qpvk42wvth468zp38nqvq2apmex5rfut3dfqm2asrsqv0tc7saqje7 --token BTC --amount 20 --use-device with "Mempool validation failed: WrapperTx signature verification failed: The wrapper signature is invalid". Same thing if I try --dry-run-wrapper.

Relatedly, do you recall why we disallow --use-device with --dry-run? We allow it with --dry-run-wrapper. (it was added in #3570)

murisi · 2024-09-26T10:52:46Z

Thanks for looking into this PR.

Relatedly, do you recall why we disallow --use-device with --dry-run? We allow it with --dry-run-wrapper. (it was added in #3570)

I believe that combining --use_device with --dry-run is disallowed because the hardware wallet does not support displaying and signing raw transactions. And I think (at least at the time when the code was written) --dry-run causes the creation of raw transactions instead of wrapper transactions.

Something is wrong with the wrapper signature. It fails on shield --source Albert --target znam1ky620tz7z658cralqt693qpvk42wvth468zp38nqvq2apmex5rfut3dfqm2asrsqv0tc7saqje7 --token BTC --amount 20 --use-device with "Mempool validation failed: WrapperTx signature verification failed: The wrapper signature is invalid". Same thing if I try --dry-run-wrapper.

If the hardware wallet produced the wrapper signature for a Tx containing a MASP Transaction, then the issue might be due to the hardware wallet's reponse. But I need to check. I'd actually been planning to add a last commit to this PR to integrate MASP hardware wallet testing with the E2E and integration tests. This will be done by Monday evening.

murisi · 2024-10-01T13:52:46Z

I'm trying to run e2e::ledger_tests::masp_txs_and_queries with these changes and 713f954:

Hi @tzemanovic . All the MASP tests now work (when using a variant of the Ledger app with the above PRs merged in) except the integration::masp::masp_fee_payment ones (failure probably due to simultaneous construction of Transactions requiring the hardware wallet) and integration::masp::expired_masp_tx (failure probably due to the time taken to press "approve" on the hardware wallet interferes with the logic of the test). Please try and run them again when you get the chance.

tzemanovic · 2024-10-03T12:38:05Z

crates/wallet/src/store.rs

@@ -63,10 +62,12 @@ pub struct ValidatorData {
 /// A Storage area for keys and addresses
 #[derive(Serialize, Deserialize, Debug, Default)]
 pub struct Store {


These changes in the wallet store are breaking, but I think we could make them compatible by e.g. trying to deserialize to the current format in case of a failure and migrate to the new format

tzemanovic · 2024-10-03T12:57:32Z

I'm trying to run e2e::ledger_tests::masp_txs_and_queries with these changes and 713f954:

Hi @tzemanovic . All the MASP tests now work...

Everything works well, thanks! But there are some affected unrelated integration tests and it looks like there's maybe a breaking change for IBC/hermes.

An observation from UX perspective - a shielded transfer and unshielding transfer require multiple wallet interactions, first to access a viewing key and then 2 more to sign the transfer(s) and again to obtain the signature from the wallet. The last 2 interactions print the same information on the wallet screen, so it appears that it's repeating the same action. I'm not sure if we can do much about that in the wallet, but I think we could at least print something out in the CLI before the request to make it clear what's happening.

I'll also add emulator automation for one of the masp tests and we'll need to update the elf file we use in the CI. For local testing I just merged together all the open PRs in ledger-namada. Am I okay to use that?

tzemanovic · 2024-10-04T10:15:19Z

One more thing - during a shielded and unshielding transfer when the amount being transferred is lower than the balance of the source, the change that stays in the sender's balance is shown as an obscured receiver payment address so it's not apparent that the destination is the same as the sender. Is there something we can do to improve this?

murisi · 2024-10-30T10:58:01Z

Everything works well, thanks! But there are some affected unrelated integration tests and it looks like there's maybe a breaking change for IBC/hermes.

You're right. I just checked, and the root causes of this were the following:

This PR changes the spending keys, viewing keys, and payment addresses used in the IBC tests from raw bech32m literals to aliases of things in the wallet. Unfortunately some of the IBC tests were still checking whether the aliases began with zvk and zsk. Furthermore literals need to be specified as receivers for IBC transactions. Both of these are now fixed.
The second cause was my breaking changes to the Store (that you mentioned above). This PR will need to be fixed to achieve backwards compatibility with the old format (as you mentioned), but regardless Hermes will need its namada_sdk dependency updated to support the new representation I think...

murisi · 2024-10-30T11:48:49Z

I'll also add emulator automation for one of the masp tests and we'll need to update the elf file we use in the CI. For local testing I just merged together all the open PRs in ledger-namada. Am I okay to use that?

This makes sense for testing the CI changes now since it might take some time before they release ZIP32 support. (They have addressed a bunch of issues in main so there are less open PRs to merge now.) But ultimately we should wait until Zondax makes an official feature-complete release elf-file before we adjust the CI. This is because they might implement something entirely different/incompatible.

murisi · 2024-10-30T12:07:42Z

One more thing - during a shielded and unshielding transfer when the amount being transferred is lower than the balance of the source, the change that stays in the sender's balance is shown as an obscured receiver payment address so it's not apparent that the destination is the same as the sender. Is there something we can do to improve this?

This is a very good point. I think we can open an issue at https://github.com/Zondax/ledger-namada to propose that the Namada Ledger app checks whether Sapling outputs are going to the default payment address of the signer, and if so annotates the output with an indication that it is actually change.

…sure removal of MASP Builder data.

…et wallet.

…eters cannot be reused. Fixed MASP integration tests depending on access to secret keys.

…sis files.

murisi · 2024-11-27T17:10:10Z

One more thing - during a shielded and unshielding transfer when the amount being transferred is lower than the balance of the source, the change that stays in the sender's balance is shown as an obscured receiver payment address so it's not apparent that the destination is the same as the sender. Is there something we can do to improve this?

This is a very good point. I think we can open an issue at https://github.com/Zondax/ledger-namada to propose that the Namada Ledger app checks whether Sapling outputs are going to the default payment address of the signer, and if so annotates the output with an indication that it is actually change.

Done that here: https://github.com/Zondax/ledger-namada-zip32/issues/7 . Thanks again for the suggestion.

murisi force-pushed the murisi/integrate-hw-masp branch from 2e32593 to 8fac040 Compare September 10, 2024 10:48

murisi marked this pull request as draft September 10, 2024 10:48

murisi force-pushed the murisi/integrate-hw-masp branch 2 times, most recently from 3f79328 to bad72be Compare September 14, 2024 14:56

murisi force-pushed the murisi/integrate-hw-masp branch 3 times, most recently from cd0decd to 3fe7ee9 Compare September 23, 2024 10:29

murisi force-pushed the murisi/integrate-hw-masp branch from 216861c to e448c1a Compare September 24, 2024 19:44

murisi requested review from cwgoes, karbyshev and tzemanovic September 24, 2024 19:45

murisi force-pushed the murisi/integrate-hw-masp branch from e448c1a to e08c8b7 Compare September 24, 2024 21:26

murisi added MASP externally blocked labels Sep 26, 2024

murisi marked this pull request as ready for review September 26, 2024 10:58

murisi force-pushed the murisi/integrate-hw-masp branch 2 times, most recently from 0332a63 to a375e6e Compare September 28, 2024 08:16

tzemanovic reviewed Oct 3, 2024

View reviewed changes

This was referenced Oct 4, 2024

hw masp auto test #3890

Open

Avoid redundant wrapper signatures #3896

Open

murisi force-pushed the murisi/integrate-hw-masp branch from 69704a0 to 7748a05 Compare October 29, 2024 13:23

murisi mentioned this pull request Oct 30, 2024

Fix signing for non-MASP IBC Transfers heliaxdev/ledger-namada#2

Open

brentstone mentioned this pull request Nov 8, 2024

feat: masp building compiles anoma/namada-interface#1221

Draft

murisi force-pushed the murisi/integrate-hw-masp branch 3 times, most recently from f857c8d to d724bde Compare November 22, 2024 08:34

murisi and others added 9 commits November 27, 2024 10:38

Implemented MASP signing using the hardware wallet.

44c5f03

Factored out the logic for MASP hardware wallet signing.

5fd0281

Expand MASP hardware wallet support to other transaction types.

b78d8bf

Separate the storage of shielded keys from their birthdays.

9e7c974

Never use the hardware wallet to sign the fee header alone. Always en…

381531b

…sure removal of MASP Builder data.

Moved the shielded keys used in the integration tests into the localn…

0ef7b66

…et wallet.

Dont do a dry run if using a device in the MASP integration tests.

2d8c41c

Always sign MASP Transactions before dumping because randomness param…

3e72fe1

…eters cannot be reused. Fixed MASP integration tests depending on access to secret keys.

test/e2e/masp: add support for testing with HW wallet

fd48de2

jurevans mentioned this pull request Nov 27, 2024

SDK: Finalize v0.11.0 Release anoma/namada-interface#1324

Open

murisi added 2 commits November 27, 2024 13:15

Sometimes use literals instead of aliases in the IBC tests.

bdd8e9f

Make shielded balance checking more robust for IBC tests.

706a143

murisi force-pushed the murisi/integrate-hw-masp branch from d724bde to e4db3ce Compare November 27, 2024 12:52

Adjust integration tests to work with both hardware and localnet gene…

24fcb3b

…sis files.

murisi force-pushed the murisi/integrate-hw-masp branch from e4db3ce to 24fcb3b Compare November 27, 2024 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Murisi/integrate hw masp #3797

Murisi/integrate hw masp #3797

murisi commented Sep 10, 2024 •

edited

Loading

Fraccaman commented Sep 18, 2024

codecov bot commented Sep 23, 2024 •

edited

Loading

tzemanovic commented Sep 26, 2024

murisi commented Sep 26, 2024

murisi commented Oct 1, 2024 •

edited

Loading

tzemanovic Oct 3, 2024

tzemanovic commented Oct 3, 2024

tzemanovic commented Oct 4, 2024

murisi commented Oct 30, 2024

murisi commented Oct 30, 2024 •

edited

Loading

murisi commented Oct 30, 2024

murisi commented Nov 27, 2024

Murisi/integrate hw masp #3797

Are you sure you want to change the base?

Murisi/integrate hw masp #3797

Conversation

murisi commented Sep 10, 2024 • edited Loading

Describe your changes

Checklist before merging

Fraccaman commented Sep 18, 2024

codecov bot commented Sep 23, 2024 • edited Loading

Codecov Report

tzemanovic commented Sep 26, 2024

murisi commented Sep 26, 2024

murisi commented Oct 1, 2024 • edited Loading

tzemanovic Oct 3, 2024

Choose a reason for hiding this comment

tzemanovic commented Oct 3, 2024

tzemanovic commented Oct 4, 2024

murisi commented Oct 30, 2024

murisi commented Oct 30, 2024 • edited Loading

murisi commented Oct 30, 2024

murisi commented Nov 27, 2024

murisi commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 23, 2024 •

edited

Loading

murisi commented Oct 1, 2024 •

edited

Loading

murisi commented Oct 30, 2024 •

edited

Loading